Addeded 403 account block status code handling for gitlab #3471
Conversation
pkg/detectors/gitlab/v1/gitlab.go
Outdated
| } | ||
| s1.ExtraData = map[string]string{ | ||
| "rotation_guide": "https://howtorotate.com/docs/tutorials/gitlab/", | ||
| "version": fmt.Sprintf("%d", s.Version()), | ||
| } | ||
|
|
||
| if verify { | ||
| isVerified, verificationErr := s.verifyGitlab(ctx, resMatch) | ||
| isVerified, isUserBlocked, verificationErr := s.verifyGitlab(ctx, resMatch) |
There was a problem hiding this comment.
Can verifyGitlab func returns ExtraData rather than just boolean ? IMO code readability will improve and will be consistent with verification func of other detectors.
There was a problem hiding this comment.
I think it makes sense to prioritize consistency across the detectors, especially since we have many of them. One other aspect to consider is that returning multiple values with the same type can cause ambiguity. It's easy to mix up with bool represents what. While named return types can reduce ambiguity, they can also introduce inconsistency if not applied uniformly across the codebase.
I wonder if there is an approach where we can use a specific Error type and perform and errors.Is check?
I'm not at my computer, otherwise I'd have tried to play around with this a bit.
There was a problem hiding this comment.
I actually thought of that first but dropped the idea thinking that extraData will be returned for just one case. Your comments make sense. I will make that change now.
pkg/detectors/gitlab/v1/gitlab.go
Outdated
| // check if the user account is blocked or not | ||
| var apiResp gitlabMessage | ||
| if err := json.Unmarshal(body, &apiResp); err == nil { | ||
| if apiResp.Message == blockedUserMessage { | ||
| return true, true, nil | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
What if we just find that string in the bodyString ? I would love to see @ahrav's input in this matter.
There was a problem hiding this comment.
String matching on errors in general makes me uneasy, but if we’re confident in the response structure and error location, it should work. Alternatively, we could use bytes.Contains to check the entire response after consuming the body. I’m not sure about the performance difference, but I suspect bytes.Contains might be slightly faster. JSON decoding in Go is typically slow due to the standard library’s use of reflection. That said, I’d go with whichever option is clearer to everyone.
There was a problem hiding this comment.
Removed unmarshalling logic
pkg/detectors/gitlab/v1/gitlab.go
Outdated
| s1.Verified = isVerified | ||
| // if user account is blocked, add this information in the extra data | ||
| if isUserBlocked { | ||
| s1.ExtraData["User Account Blocked"] = "Yes" |
There was a problem hiding this comment.
This is a bit wordy. I think just "blocked = true" would be fine.
https://docs.gitlab.com/ee/api/users.html#as-a-regular-user
e.g.,
trufflehog/pkg/detectors/github/v1/github_old.go
Lines 152 to 154 in 871a2b0
kashifkhan0771
force-pushed
the
update/gitlab-403-status-handling
branch
from
92e9bfe to
294a2fe
Compare
* main: (76 commits) update aws descriptions (trufflesecurity#3529) enforce timeout on circleci test (trufflesecurity#3528) rm snifftest (trufflesecurity#3527) Redact more source credentials (trufflesecurity#3526) Create global log redaction capability (trufflesecurity#3522) Adding basic "what is trufflehog" to the readme (trufflesecurity#3514) Handle custom detector response and include in extra data (trufflesecurity#3411) fix: fixed validation logic for `calendarific` (trufflesecurity#3480) fix(deps): update github.com/tailscale/depaware digest to 3d7f3b3 (trufflesecurity#3518) Move DecoderType into ResultWithMetadata trufflesecurity#3502 Addeded 403 account block status code handling for gitlab (trufflesecurity#3471) updated gcpapplicationdefaultcredentials detector results with RawV2 (trufflesecurity#3499) fix(deps): update module github.com/brianvoe/gofakeit/v7 to v7.1.1 (trufflesecurity#3512) fix(deps): update module github.com/schollz/progressbar/v3 to v3.17.0 (trufflesecurity#3510) fix(deps): update module cloud.google.com/go/secretmanager to v1.14.2 (trufflesecurity#3498) Adds a logging section in the contributing guidelines (trufflesecurity#3509) fix: fixed verifcation pattern logic for `bulksms` (trufflesecurity#3478) Extend `algoliaadminkey` with additional checks (trufflesecurity#3459) fix(deps): update module google.golang.org/api to v0.203.0 (trufflesecurity#3497) fix: added correct api endpoint for verification & logic for Aeroworkflow (trufflesecurity#3435) ...
Description:
This PR handle 403 status code for Gitlab detector and verify if the user account is blocked or not and add that information in the Extra Data
JIRA Ticket:
CSM-637
Checklist:
make test-community)?make lintthis requires golangci-lint)?